feat: add sequential batch support #5762
Conversation
vinistevam
force-pushed
the
feat/add-sequential-batch-hook
branch
from
0e732db to
8b9e6f7
Compare
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
| @@ -332,7 +337,8 @@ async function getNestedTransactionMeta( | |||
| async function addTransactionBatchWithHook( | |||
| request: AddTransactionBatchRequest, | |||
| ): Promise<TransactionBatchResult> { | |||
| const { publishBatchHook, request: userRequest } = request; | |||
| const { publishBatchHook: initialPublishBatchHook, request: userRequest } = | |||
There was a problem hiding this comment.
Minor, would this be more explicit as requestPublishBatchHook?
| TransactionReceipt, | ||
| } from '../types'; | ||
|
|
||
| const TRANSACTION_CHECK_INTERVAL = 5000; // 5 seconds |
There was a problem hiding this comment.
Ideally this would also use the dynamic intervals from LaunchDarkly, so maybe it is cleaner to have a callback such as getPendingTransactionTrackerByChainId and then ask a tracker instance directly to poll our transaction?
|
|
||
| const log = createModuleLogger(projectLogger, 'sequential-publish-batch-hook'); | ||
|
|
||
| type SequentialPublishBatchHookParams = { |
There was a problem hiding this comment.
Minor, maybe SequentialPublishBatchHookOptions?
| * @returns The hook function. | ||
| */ | ||
| getHook() { | ||
| return async ({ |
There was a problem hiding this comment.
Rather than a function returning another function, should we use the pattern of having a normal #hook method that we bind here to keep it as readable as possible?
| for (const transaction of transactions) { | ||
| try { | ||
| const transactionMeta = this.#getTransaction(String(transaction.id)); | ||
| const transactionHash = await this.#publishTransaction( |
There was a problem hiding this comment.
As the logic is so small here, I wonder if it's simpler for the hook to directly call sendRawTransaction via the EthQuery?
| ): Promise<boolean> { | ||
| let attempts = 0; | ||
|
|
||
| while (attempts < MAX_TRANSACTION_CHECK_ATTEMPTS) { |
There was a problem hiding this comment.
Is it out of our scope to define a timeout here, as it's so variable based on chain, and gas fees etc?
| } | ||
|
|
||
| const waitPromise = new Promise((resolve) => | ||
| setTimeout(resolve, TRANSACTION_CHECK_INTERVAL), |
There was a problem hiding this comment.
Rather than iterating and waiting ourselves, could we add methods to the PendingTransactionTracker such as addTransaction to force a new ID into the queue? And then maybe emit an event we wait for here inside a new Promise?
The goal being to encapsulate as much as possible in the PendingTransactionTracker and just add a job to it from here.
|
|
||
| if (!isConfirmed) { | ||
| throw new Error( | ||
| `Transaction ${transactionHash} failed or was not confirmed.`, |
There was a problem hiding this comment.
Maybe Batch transaction failed - ${transactionHash} ?
Explanation
The
TransactionControllercurrently lacks support for sequential batch transactions, which are required for the stablecoin lending feature. Specifically, there is no mechanism to execute multiple transactions (e.g., approval + token deposit) sequentially while ensuring confirmation for each transaction. This limitation prevents efficient batch processing to support new features.What solution do these changes offer?
The
SequentialPublishBatchHookintroduces a default mechanism for handling batch transactions when no custompublishBatchHookis provided. It ensures transactions are published sequentially, waiting for confirmation before proceeding to the next. If any transaction fails to publish or confirm, the batch process halts and throws an error.Key Features:
References
Fixes https://github.com/MetaMask/MetaMask-planning/issues/4695
Changelog
Checklist